-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[Wasm RyuJit] throw helper / null check preliminaries #123053
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
On Wasm null checks must be explicit and exceptions raised via helper call. Set up some of the mechanism we'll need for this: * Add null check special code kind * Track Wasm ACD entries by handler region only (instead of by try or handler). The code address of the helper cannot be used in Wasm to infer EH region containment; we will use use the virtual IP for that. So we need at most one throw helper (per kind) in each funclet and in the main method region. * Ensure throw helper blocks have Wasm labels that are always on the stack in their regions by putting the throw helpers at the end of the region RPO and pretending there is a branch from the region entry. * Add plausible codegen for GT_NULLCHECK
|
FYI @dotnet/jit-contrib (still WIP, need to see how much of this is testable) See #123021 (comment) for some context. |
|
I don't have the code to make |
SingleAccretion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the code to make SCK_NULLCHECK demands in place yet. Seeing as nullchecks get generated in many places it seems less than ideal to track all these sites all down and add code to each one. I wonder if we can just do this in lower or similar.
+1. In fact I don't see the point of this two-phase setup that exists. Why not add all the blocks late (in stack setter or lower) and remove the handling from morph?
src/coreclr/jit/fgbasic.cpp
Outdated
| bool Compiler::fgUseThrowHelperBlocks() | ||
| { | ||
| #if defined(TARGET_WASM) | ||
| return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a strong reason to make WASM special here. Unique "native" code addresses still matter for "native" stacks produced by engines. It is helpful to make them (more) useful with debug code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it is not so hard -- if we have say GT_NULLCHECK(x) we can produce
block
local.get x
i32.const null-limit-value
i32.gt
br_if 0
call <helper-idx> ; THROW NULL CHECK
unreachable ;
end
and this will properly nest wherever we happen to emit it, even if we've pended other operands already. The main difference being we need to know a bit earlier if there's a common helper block we can use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jiterpreter did every check inline like this and it worked okay. What does it look like to preserve the current IP for stackwalking in this model though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically the runtime "knows" that throws from helpers should be attributable the caller's IP, not to the throw helper's IP.
When we use a common throw helper (which we only do when optimizing) we lose the ability to pin down where in the method the call to the throw came from; this is deemed an acceptable tradeoff.
I don't understand Wasm debugging well enough to know what this is going to look like for Wasm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking through the scenario where we have a dedicated block for nullref throws. When we branch to that block to do a throw, how does stackwalking determine what instruction threw the nullref? Or are we not going to have line number info in stacktraces on wasm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't know the exact answer, but I think it is as follows:
For debuggable code we will have one throw helper call per site that can cause an exception, so the source positions will be accurate if the debugger can process the various mappings (wasm offset -> IL offset -> source [the latter two possibly composed at build time and represented as DWARF? I am not clear on this]). For optimized code we won't have an accurate wasm offset -> IL offset map so can't give an accurate source position.
But this is also true with many other optimizations, eg inlining / cse / hoisting...
src/coreclr/jit/codegenwasm.cpp
Outdated
| { | ||
| assert(compiler->fgUseThrowHelperBlocks()); | ||
| genConsumeOperand(tree->Addr()); | ||
| GetEmitter()->Ins(INS_i32_eqz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ref #123053 (comment) for what value this should check against.
src/coreclr/jit/codegenwasm.cpp
Outdated
| void CodeGen::genCodeForNullCheck(GenTreeIndir* tree) | ||
| { | ||
| assert(compiler->fgUseThrowHelperBlocks()); | ||
| genConsumeOperand(tree->Addr()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| genConsumeOperand(tree->Addr()); | |
| genConsumeAddress(tree->Addr()); |
src/coreclr/jit/flowgraph.cpp
Outdated
| // For WASM we want one throw helper per funclet | ||
| // So we ignore any try region nesting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also be hardcoding the "one catch per funclet (with nested trys)" scheme, right? Otherwise we still need the nesting for proper resumption.
Apart from that, the comment could be improved to talk about "why" we want this.
I think this is just a long-standing practice that we've never reexamined. Perhaps it is time. |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
|
The inline throw helpers require a bit of coordination since we have to emit a block/end wrapper around the whole thing (so we need to know in advance if we're using throw helpers or not). So I have been working up the divide by zero checks to see how to best generalize the throw helper expansions we'll need. There (whether we use inline or common throw helpers) we need to dup an operand, but Wasm has no Any thoughts on how to best go about having a pool of temps we can use? For the (MinInt/-1) case we need access to the dividend as well. So we might also consider adding these checks explicitly in lower. |
I was thinking we'd reuse the internal register mechanism for that. It's ifdef-ed out currently though, so that will need to be fixed. |
|
@SingleAccretion is this last commit what you had in mind for the unchecked offset? I may push for merging this more or less in its current form and then revisit once #123044 is there to hook up the calls. Still todo, likely again as separate PRs:
|
Yes. Though I think the comparison should be unsigned? |
Yep, changed this. |
|
@dotnet/jit-contrib think this is worth getting in as is, we will need to revisit once some supporting pieces are in place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR establishes infrastructure for WebAssembly (Wasm) RyuJIT to handle explicit null checks and exception throwing via helper calls. Unlike other platforms where null checks can be implicit via OS page fault mechanisms, Wasm requires explicit null checks with exceptions raised through helper calls.
Changes:
- Added
SCK_NULL_CHECKspecial code kind for null reference exceptions on Wasm - Modified ACD (Add Code Descriptor) tracking to use handler regions only on Wasm (instead of try or handler regions)
- Ensured throw helper blocks have proper Wasm labels by positioning them at region end and treating them as successors of region entries
- Implemented codegen for
GT_NULLCHECKnodes with both throw helper block and inline variants
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/jitinterface.h | Updated MAX_UNCHECKED_OFFSET_FOR_NULL_OBJECT to 1023 for Wasm (smaller than other platforms) |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | Added Wasm-specific logic for maxUncheckedOffsetForNullObject in AOT compiler |
| src/coreclr/jit/gentree.h | Added SCK_NULL_CHECK enum value for null check special code kind |
| src/coreclr/jit/flowgraph.cpp | Added SCK_NULL_CHECK handling throughout exception infrastructure, modified bbThrowIndex for Wasm ACD tracking, fixed typos in comments |
| src/coreclr/jit/stacklevelsetter.cpp | Added GT_NULLCHECK case to register throw helper blocks for Wasm |
| src/coreclr/jit/fgwasm.h | Modified successor enumeration to treat ACD blocks as successors of region entries for proper Wasm control flow |
| src/coreclr/jit/compiler.cpp | Added cross-replay support for Wasm null object offset |
| src/coreclr/jit/codegenwasm.cpp | Implemented null check codegen with comparison against max unchecked offset; added placeholder for divide-by-zero checks |
| src/coreclr/jit/codegen.h | Added inst_JMP overload with isTempLabel parameter for Wasm |
| (32 * 1024 - 1) : (pEEInfoOut.osPageSize / 2 - 1); | ||
| if (_compilation.NodeFactory.Target.IsWasm) | ||
| { | ||
| pEEInfoOut.maxUncheckedOffsetForNullObject = 1024 - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This represents maximum offset that gets handled by hardware throwing access violation exceptions.
How is this value used on wasm? I would expect this to be 0 on wasm since we are not going to depend on "hardware" to throw exceptions for all cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(In any case, it would be useful to make the comment in corinfo.h more descriptive.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some discussion in a related pr #123021 (comment) which refers to an open issue in NAOT-LLVM dotnet/runtimelab#3127.
I agree that it seems counterintuitive on Wasm to have a value other than 0. The plan is to get there eventually. I can make it clearer this is expected to be an interim state as we bring up the support in the JIT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the JIT not able to deal with this value being 0 at the moment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just tried to run spmi diffs for a single collection (libraries.pmi) for maxUncheckedOffsetForNullObject being 0 on win-x64:
[02:21:08] 139,498 contexts with diffs (648 size improvements, 138,402 size regressions, 448 same size)
[02:21:08] (265 PerfScore improvements, 138,500 PerfScore regressions, 733 same PerfScore)
[02:21:08] -9,075/+945,747 bytes
[02:21:08] -2.84%/+29.11% PerfScore
quite a massive regression 😐
it's 32767 by default for me, but 1024 handles 99% of the regressions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the JIT not able to deal with this value being
0at the moment?
As far as I know 0 works" just fine (modulo possible latent bugs and the code size bloat).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quite a massive regression
Right, that's expected. The explicit null checks for wasm need work to be reasonably efficient.
We typically go for correctness first during bring ups and optimize later. 0 is the only correct value for wasm. I understand that it produces inefficient code, but that is expected at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 is the only correct value for wasm
Since with explicit null checks, there are no "OS details" involved, we can choose any value we like that is < __global_base (1024 by default for optimized link - we will need to set it explicitly for non-optimized link). I agree that in the long term 0 is "the best" value, since the code sequences are smallest with it.
However, at this present point it will be both less efficient and (perhaps more importantly) less correct. Less correct because (among corner case bugs I am either not aware of or forgot about), we have a number of places in the Jit where we produce "naked" IND(x + 8)-like trees (for things like x.Length and such) and expect the implicit null-checking to kick in.
On Wasm null checks must be explicit and exceptions raised via helper call.
Set up some of the mechanism we'll need for this: